-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Jaeger exporter with nil values #490
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drogus thanks for the PR here.
I think this generally lgtm, nil
values generally shouldnt be allowed in span attributes was my understanding though, https://github.com/open-telemetry/opentelemetry-specification/blob/82865fae64e7b30ee59906d7c4d25a48fe446563/specification/common/common.md#attributes, so i'm curious how we've got to this state.
That being said I think it's good to be defensive here.
I think once you sign the CLA i can approve
It happens because we're not checking parameters here: opentelemetry-ruby/sdk/lib/opentelemetry/sdk/trace/span.rb Lines 69 to 82 in 659be35
opentelemetry-ruby/sdk/lib/opentelemetry/sdk/trace/span.rb Lines 272 to 280 in 659be35
|
If that's the case I think a better fix would be to make sure that they're disallowed in the spans, cause my fix is just for the Jaeger exporter. If another exporter also makes the assumption that #207 looks interesting, but I see it's quite old, so would it be fine if I created a smaller PR that returns early from |
👍 I think that's good for now. I will be working to improve our error handling story in #465 and we can decide then whether we want to allow devs to optionally |
Sorry for no activity here. I'll work on a fix, but there's also another problem that I've found - it will also fail when the value can't be mapped to one of the thrift values (like any Ruby class other than |
It is supposed to only allow:
We should log and ignore any other values. |
I just ran into this while instrumenting some code and was curious if there has been any progress since the last update. |
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
When there's a
nil
value in the span data attributes Jaeger exporter will fail with a following error:unexpected error in Jaeger::CollectorExporter#export - Unknown key given to OpenTelemetry::Exporter::Jaeger::Thrift::Tag.new:
. It's unexpected and hard to track down as the error does not even give the key name. It's also quite common in Ruby apps to not check fornil
values when setting them as an attribute - a common practice is to either conver them to empty strings or ignore them.This PR ignores
nil
attributes, but an alternative (empty strings) could also be implemented.